-
Notifications
You must be signed in to change notification settings - Fork 46
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
[CB-14140] Replace shelljs calls with fs-extra & which #21
Conversation
Codecov Report
@@ Coverage Diff @@
## master #21 +/- ##
==========================================
+ Coverage 85.51% 85.63% +0.11%
==========================================
Files 19 19
Lines 1761 1747 -14
Branches 371 367 -4
==========================================
- Hits 1506 1496 -10
+ Misses 255 251 -4
Continue to review full report at Codecov.
|
061db2d
to
d84c28a
Compare
👍 I thought about proposing the same substitution when working on |
I have all tests passing here, but we still use shelljs in one spot (superspawn for The potential concern is that these changes will cause test failures in other modules because they are stubbing methods on |
@dpogue Regarding stubbing/mocking: Use this to get rid of shelljs? https://www.npmjs.com/package/which |
This should be ready for review now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks like a correct transformation of the existing code to me 👍
I left some suggestions on how to further improve some of the locations you touched. Some might be considered out of the scope of this PR.
@@ -196,10 +195,10 @@ describe('config-changes module', function () { | |||
|
|||
describe('processing of plugins (via process method)', function () { | |||
beforeEach(function () { | |||
shell.cp('-rf', dummyplugin, plugins_dir); | |||
fs.copySync(dummyplugin, path.join(plugins_dir, path.basename(dummyplugin))); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Since this idiom occurs quite often in this file, I'd add a helper like this
function installPlugin (pluginPath) {
fs.copySync(pluginPath, path.join(plugins_dir, path.basename(pluginPath)));
}
spec/CordovaCheck.spec.js
Outdated
@@ -31,12 +31,12 @@ describe('findProjectRoot method', function () { | |||
process.chdir(cwd); | |||
}); | |||
function removeDir (someDirectory) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'd inline all occurrences of this helper and drop it, since it's now only an alias
Edit: I realize now that the placement of the comment was suboptimal
spec/CordovaCheck.spec.js
Outdated
} | ||
it('Test 001 : should return false if it hits the home directory', function () { | ||
var somedir = path.join(home, 'somedir'); | ||
removeDir(somedir); | ||
shell.mkdir(somedir); | ||
fs.ensureDirSync(somedir); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this particular instance, one could use fs.emptyDirSync(somedir)
to replace the remove/create idiom
spec/CordovaCheck.spec.js
Outdated
shell.mkdir('-p', path.join(anotherdir, 'www', 'config.xml')); | ||
shell.mkdir('-p', path.join(somedir, 'www')); | ||
shell.mkdir('-p', path.join(somedir, 'config.xml')); | ||
fs.ensureDirSync(anotherdir); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We don't need this because of next line
Again, bad placement of the comment on my part 🙄
spec/CordovaCheck.spec.js
Outdated
shell.mkdir('-p', anotherdir); | ||
shell.mkdir('-p', path.join(somedir, 'www', 'config.xml')); | ||
fs.ensureDirSync(anotherdir); | ||
fs.ensureDirSync(path.join(somedir, 'www', 'config.xml')); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think the previous version was just a lazy hack. We actually want fs.ensureFileSync
on all instances of this. One might might even consider adding a test that asserts that directories are not accepted as config.xml
.
src/FileUpdater.js
Outdated
} else if (!targetStats.isDirectory() && sourceStats.isDirectory()) { | ||
log('delete ' + targetPath + ' (source is a directory)'); | ||
shell.rm('-f', targetFullPath); | ||
if ((targetStats.isDirectory() && !sourceStats.isDirectory()) || (!targetStats.isDirectory() && sourceStats.isDirectory())) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Condition is equivalent to targetStats.isDirectory() !== sourceStats.isDirectory()
.
I'd merge with containing block to targetStats && (targetStats.isDirectory() !== sourceStats.isDirectory())
src/PlatformJson.js
Outdated
@@ -37,7 +36,7 @@ PlatformJson.load = function (plugins_dir, platform) { | |||
}; | |||
|
|||
PlatformJson.prototype.save = function () { | |||
shelljs.mkdir('-p', path.dirname(this.filePath)); | |||
fs.ensureDirSync(path.dirname(this.filePath)); | |||
fs.writeFileSync(this.filePath, JSON.stringify(this.root, null, 2), 'utf-8'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This can be simplified to:
fs.outputJsonSync(this.filePath, this.root, {spaces: 2})
Encoding defaults to utf-8
src/PlatformJson.js
Outdated
@@ -212,7 +211,7 @@ PlatformJson.prototype.generateMetadata = function () { | |||
*/ | |||
PlatformJson.prototype.generateAndSaveMetadata = function (destination) { | |||
var meta = this.generateMetadata(); | |||
shelljs.mkdir('-p', path.dirname(destination)); | |||
fs.ensureDirSync(path.dirname(destination)); | |||
fs.writeFileSync(destination, meta, 'utf-8'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Again, these lines can be replaced by:
fs.outputFileSync(destination, this.generateMetadata());
Thanks for those review comments @raphinesse! Since they all help to make the code easier to read I'll consider them all in scope 😉 I've added another commit addressing them. We'll squash this branch when merging. |
Did you include the async test handling changes by accident? Since they are completely unrelated, I would consider them out of scope 😉 I'm meaning to say the async test handling changes are fine but should go in a separate PR IMHO. |
Fixed the re-spacing JSON output, and pulled out the spec changes for a future PR. Thanks! |
src/PlatformJson.js
Outdated
@@ -211,8 +210,7 @@ PlatformJson.prototype.generateMetadata = function () { | |||
*/ | |||
PlatformJson.prototype.generateAndSaveMetadata = function (destination) { | |||
var meta = this.generateMetadata(); | |||
fs.ensureDirSync(path.dirname(destination)); | |||
fs.writeFileSync(destination, meta, 'utf-8'); | |||
fs.outputJsonSync(destination, meta); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hate to bug you again with this, but it was not about re-spacing JSON output. generateMetadata
returns a string containing JS, not JSON. Therefore we need fs.outputFileSync(destination, meta)
here (mind the File).
I also wrote a butt-ugly test that will fail with your implementation:
describe('generateAndSaveMetadata method', function () {
it('should save generated metadata', function () {
// Needs to use graceful-fs, since that is used by fs-extra
const spy = spyOn(require('graceful-fs'), 'writeFileSync');
const dest = require('path').join(__dirname, 'test-destination');
platformJson.addPluginMetadata(fakePlugin).generateAndSaveMetadata(dest);
expect(spy).toHaveBeenCalledTimes(1);
const [file, data] = spy.calls.argsFor(0);
expect(file).toBe(dest);
expect(data.indexOf(JSON.stringify(platformJson.root.modules, null, 2))).toBeGreaterThan(0);
expect(data).toMatch(JSON.stringify(platformJson.root.plugin_metadata, null, 2));
});
});
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ahh, now I understand! 🤦♂️
I've fixed that and added your test to the PlatformJson.spec.js
Any timeline when this one will be merged? |
I'm hoping to do a minor version release of cordova-common with bugfixes before merging this, since this might qualify as a major-version API change. |
We've done the cordova-common 2.2.3 release, I think we're ready to merge this. |
With the greatest of pleasure 😁 |
Linking this wonderful change which landed in d781ffd to CB-14140 (https://issues.apache.org/jira/browse/CB-14140). |
Replaces all uses of
shelljs
withfs-extra
for handling directory creation/deletion and file copying. Replaces the single use ofshelljs.which
withwhich
.While this doesn't have a huge impact here, fs-extra should give us some more stability in our tests in other projects like cordova-lib, especially around directory deletions on Windows.
The potential concern is that these changes will cause test failures in other modules because they are stubbing methods on
fs
instead offs-extra
, but my hope would be to update all those other modules as well. I've got a branch of cordova-lib that I've started updating.